Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[12.x] Const to enum #53961

Closed
wants to merge 43 commits into from
Closed

[12.x] Const to enum #53961

wants to merge 43 commits into from

Conversation

shaedrich
Copy link
Contributor

Maybe, it's time for Laravel to not only allow custom enums but also to have it's own

  • Moved enumerable class constants to separate enums
  • Tried to stay backward compatible by
    • Deprecating the constants instead of removing them
    • Allowing to pass both the enum and the underlying value to a function and wrap the argument in an enum_value() call

trait ManagesFrequencies
{
/**
* The Cron expression representing the event's frequency.
*
* @param string $expression
* @param string|\Illuminate\Console\Enums\ScheduleOn $expression
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this method even supposed to accept single integers like ->cron(3)? I thought it's intended for those cron strings like 0 15 10 ? * 6L 2002-2005.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this change supposed to work when replacing the underlying cron string with a single number?

Maybe instead of changing this method, we could add a test case to ensure this method only accepts cron strings?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided to add validation in a separate PR: #53971

@tontonsb
Copy link
Contributor

Regarding casing... AFAIK the vast majority of devs use editors with syntax highlighting. At least Notepad++ or Vim. So there's no need to set apart constants/enum cases by uppercasing them. Although the uppercase practice is too engrained for constants, we might consider not continuing it for enums. Wouldn't PasswordStatus::passwordReset or PasswordStatus::PasswordReset be nicer?

Overall enums are intended for finite sets of values you're willing to accept and handle, they make an unextensible list of values. If the values are just "some values" or even worse "some of the values", constants or plain classes should be used instead. Enums are not a different syntax for constants, they are types. I.e. public function myfun(HandledCase $case) should be used if you would otherwise do this:

public function myfun(string $case)
{
    if (!in_array($case, static::handledCases))
        throw new \DomainException("Invalid case value $case here.");

    // ....
}

See also https://peakd.com/hive-168588/@crell/on-the-use-of-enums

Do we want to limit any of these into a final, non-extensible list of cases? Even for weekdays (which afaik make a finite list per se) we're talking about the weekday portion of a cron string which should accept 0, 7, 2-6, tue-fri, 4#2, 5L. I think in all of those cases constants were the appropriate choice as aliases/sugar over the most common values.

@taylorotwell
Copy link
Member

Agree with casing comment. I would only change Password reset and Day scheduling stuff to Enums. The worker stuff shouldn't be changed imo. It's not user facing.

@taylorotwell taylorotwell marked this pull request as draft December 18, 2024 18:50
@shaedrich
Copy link
Contributor Author

@taylorotwell Thanks for the clear instructions—I'll change that 👍🏻

shaedrich added a commit to shaedrich/framework that referenced this pull request Dec 18, 2024
@shaedrich shaedrich marked this pull request as ready for review December 18, 2024 23:02
@shaedrich
Copy link
Contributor Author

shaedrich commented Dec 19, 2024

Even for weekdays (which afaik make a finite list per se) we're talking about the weekday portion of a cron string which should accept 0, 7, 2-6, tue-fri, 4#2, 5L. I think in all of those cases constants were the appropriate choice as aliases/sugar over the most common values.

@tontonsb As far as I can see, they are not type-hinted anywhere in a way that would restrict you to pass anything else. They currently only appear to my knowledge in places where these are passed directly. You can still pass anything you like to ManagesFrequencies::days()

@tontonsb
Copy link
Contributor

As far as I can see, they are not type-hinted anywhere in a way that would restrict you to pass anything else.

@shaedrich yeah, but type-hinting in a way that would restrict you to pass anything else is the reason why enums exist. They are types, not a syntax for magic values (that's the job of constants). If you're not going to limit the list of values, enums are not the right tool.

@shaedrich
Copy link
Contributor Author

shaedrich commented Dec 19, 2024

@tontonsb I think, we are discussing two separate things here. That might be why we talked slightly past each other.

  • Replacing (non-finite) constants with (finite) enums
  • Using enums as (restricting/enforcing) type hints (doesn't happen in this PR, methods still accept int, for example)

So, the former is something, we probably have to wait for Taylor's opinion.

@tontonsb
Copy link
Contributor

@shaedrich I think I understand your point, I'm just trying to say that the former is a usecase for constants not enums. Only the latter is for enums.

If you don't want a restricting type, but want the list to be finite, that is still a job for constants. Here's an example from the same article I linked earlier:

final class Role
{
   public const Admin = 'admin';
   public const Editor = 'editor;
   public const User = 'user';
   public const AnonymousCoward = 'anonymous';
}

You probably know that already, but for future readers — the author of that article is not some random enum hater. He introduced enums in PHP himself.

@shaedrich
Copy link
Contributor Author

@shaedrich I think I understand your point, I'm just trying to say that the former is a usecase for constants not enums. Only the latter is for enums.

@tontonsb I agree: Constants are no enums and both have their respective place. However, the question remains: Should this even be a non-finite list in the first place?

@taylorotwell
Copy link
Member

Renamed ScheduleOn to Day and PasswordStatus to PasswordResetResult.

@taylorotwell
Copy link
Member

Actually keeping PasswordResetResult as constants on a class. I like the camel casing and better naming, but don't want the breaking change of switching to an enum, which is not a string.

@taylorotwell
Copy link
Member

Eh, imo, this thing is getting a bit overcooked. I added camel case constants for password reset on 11.x.

If you want to separately PR a simple Day enum go for it. I don't see why it couldn't go to 11.x.

@shaedrich
Copy link
Contributor Author

shaedrich commented Dec 19, 2024

see #53983 and #53961

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants